-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Delete assertThrows() #1396
Delete assertThrows() #1396
Conversation
The Javadoc of |
…eption. JUnit 5 decided to make the same change in the Assertions class, so this change makes the JUnit4 API consistent.
2fd6312
to
02a7800
Compare
Fixed comment that referenced |
Thanks, Marc. I am going to wait for one or two more approvals. |
I also requested feedback on the original pull request. |
Is there a single precedent anywhere in JUnit for an |
@rschmitt see junit-team/junit5#531 for the discussion about the associated JUnit 5 APIs. |
@rschmitt did you have a chance to read that thread? Personally I don't have a strong opinion about how these methods should be named (the name |
The only new point I encountered in the other thread is that "expect" terminology has different connotations in some other frameworks. The only specific example of that was Google Test, which is actually a C++ testing framework. Since we're drawing analogies to non-Java languages, I'll point out the https://doc.rust-lang.org/std/option/enum.Option.html#method.expect There is another argument I saw which I would like to directly refute, this one from @sbrannen:
To this I respond: so what? The key point here is that |
cc11427
to
056af41
Compare
056af41
to
2fb2db1
Compare
Regardless of the name in JUnit5, I'm coming around to the decision that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regardless of the name in JUnit5, I'm coming around to the decision that
expectThrows
is a better name for JUnit4 (see the discussion on the JUnit5 thread)
I strongly disagree. I have done quite a few talks in the last couple of months and often got the feedback that expectThrows should be called assertThrows.
In terms of naming conventions, this strikes me as the equivalent of creating a function called
toString
that writes something to standard out and returnsvoid
instead ofString
.
I see that differently. The name assertThrows
is perfectly fine because it does assert that an exception is thrown. Therefore, it does match the other methods in Assert
, it just returns the actual exception as additional info.
The method executes a lambda, catches the thrown exception, asserts that the exception's type matches the passes-in class, then returns the caught exception; the assertion is only one small part So it is quite different than all the other methods. |
There is no harm in having both like in TestNG, for ~1.5 years already. |
@panchenko what specifically in TestNG are you referring to? |
… On Wed, Dec 14, 2016 at 12:32 PM, Kevin Cooney ***@***.***> wrote:
@panchenko <https://github.com/panchenko> what specifically in TestNG are
you referring to?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1396 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADjW-a6vokX8FNcMovuWzYrTB0C9WMwIks5rIFJMgaJpZM4LAdYF>
.
|
So @panchenko you are proposing keeping both I wasn't aware that the same methods were added to TestNG. Perhaps we should reach out to the TestNG team to see if the methods they had led to any confusion, and if not, keep both methods (at least in |
I disagree that Looking at TestNG's source code (and remembering the source code from older versions of JUnit 5), it looks to me that |
@jbduncan actually I don't follow the performance concerns. These methods are passed lamdas which throw exceptions. Exception handling is much more expensive than a function call. JUnit4 doesn't have any |
@kcooney Oh yes, silly me! Got them a bit mixed in my mind. |
Delete expectThrows(). Change assertThrows() to return the caught exception.
JUnit 5 decided to make the same change in the Assertions class, so this
change makes the JUnit4 API consistent.